Pull request overview

This PR tightens SSRF-related URL validation in the OpenAPI REST operation runner by enforcing path-boundary matching when comparing resolved request URLs against configured AllowedBaseUrls.

Changes:

• Hardened RestApiOperationRunner.ValidateUrl() to avoid accidental prefix matches (e.g., /v1 matching /v1-evil).
• Added unit tests to cover the prefix-collision block case and to confirm valid sub-paths remain allowed.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Automated Code Review

Reviewers: 4 | Confidence: 93%

✓ Correctness

This is a well-crafted security fix for a path-prefix collision vulnerability in URL validation. The original StartsWith check allowed URLs like https://api.example.com/v1-evil to pass validation when https://api.example.com/v1 was the allowed base URL. The fix normalizes the base URL to include a trailing slash before the prefix check, and adds an exact-match branch to still allow requests to the base URL itself (without a trailing slash). The two new tests correctly cover the attack scenario and the legitimate sub-path scenario. The logic is sound and I found no correctness issues.

✓ Security Reliability

This is a well-crafted security fix that closes a real URL prefix-collision vulnerability in the AllowedBaseUrls validation. Before this change, https://api.example.com/v1-evil would pass validation against an allowed base URL of https://api.example.com/v1 via naive string prefix matching. The fix normalizes the base URL to end with / before prefix comparison and adds an exact-match branch for the no-trailing-slash case. Both branches are necessary and correct. The two new tests cover the blocking and allowing scenarios precisely. No security or reliability issues found.

✓ Test Coverage

The security fix for path prefix collision is correct and the two new tests cover the primary attack vector (path-level prefix collision) and the legitimate sub-path case. However, the newly added exact-match code path (string.Equals(urlString, baseUrl.AbsoluteUri, ...) at line 256) has no dedicated test. This branch is essential for when the request URL exactly equals the allowed base URL with no additional path (e.g., both are https://api.example.com/v1), since the slash-appended StartsWith check would reject it. Without a test, this branch could regress silently. Additionally, a host-level prefix collision test (e.g., api.example.com allowed vs api.example.com.evil.com requested) would strengthen the security regression suite.

✗ Design Approach

The change fixes the specific /v1 vs /v1-evil collision, but it does so with another string-prefix special case instead of validating URI components. Because the runner validates the fully built request URL after appending query parameters, the new logic will now reject legitimate requests to the exact allowed resource when that resource has a query string (for example https://api.example.com/v1?foo=bar). That makes the approach too narrow for the contract this option is meant to enforce.

Suggestions

• Consider adding a host-level prefix collision test where the allowed base URL is https://api.example.com and the malicious server is https://api.example.com.evil.com. While Uri.AbsoluteUri normalization currently protects against this, an explicit test would guard against future regressions.
• Add a regression test for an allowed base URL used as the exact resource with query parameters (e.g., allowed base https://api.example.com/v1, request https://api.example.com/v1?user=1) to exercise validation against the URL shape that BuildsOperationUrl actually produces.

Automated review by SergeyMenshykh's agents